fix "resource has children" trap due to undropped pollables #63
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, it was possible to drop an `input-stream`, `output-stream`, or `future-incoming-response` while it still had an undropped child `pollable` given that `spin-executor` provided no way to cancel a `pollable` previously registered using `push_waker`. In addition, it was possible to unintentionally register more than one pollable for such a resource concurrently. This commit adds a `CancelToken` return value to `push_waker`, which may be used to explicitly cancel the registration and immediately drop the `pollable`. We now use that, along with `CancelOnDropToken`, to ensure that we cancel any previous registration before creating a new one, as well as guarantee that the registration is cancelled before attempting to drop the parent resource. Signed-off-by: Joel Dice <[email protected]>
alexcrichton
approved these changes
Apr 15, 2025
radu-matei
approved these changes
Apr 15, 2025
Member
radu-matei
left a comment
radu-matei
left a comment
There was a problem hiding this comment.
Tested this in my scenario that was failing and things look good!
Thank you!
LGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, it was possible to drop an
input-stream,output-stream, orfuture-incoming-responsewhile it still had an undropped childpollablegiven thatspin-executorprovided no way to cancel apollablepreviously registered usingpush_waker. In addition, it was possible to unintentionally register more than one pollable for such a resource concurrently.This commit adds a
CancelTokenreturn value topush_waker, which may be used to explicitly cancel the registration and immediately drop thepollable. We now use that, along withCancelOnDropToken, to ensure that we cancel any previous registration before creating a new one, as well as guarantee that the registration is cancelled before attempting to drop the parent resource.